-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display transcript text and follow along the audio #7103
Display transcript text and follow along the audio #7103
Conversation
Nice. A suggestion after looking at the screenshot. I think text should be justified to make it easier to read. |
@Matth7878 Full justified text is only acceptable when hyphenation is applied correctly. Otherwise you get 'rivers' (vertical lanes of white space) which aren't great, especially for folks with dyslexia. I don't know if good hyphenation (which of course is langue-dependant) is doable in Android Some resources on this: |
@tonytamsf Nice! I think I'd put it in a full window, though, instead of a modal. Gives just that little extra space, and with a top bar we get a logical place to add a search icon (in future). |
@keunes Thoughts about dialog vs full fragment view @ByteHamster ? |
Hi @tonytamsf
Not in any different way from a dialog, I would say. (I believe we did discuss having player controls but potentially in future.) Maybe a full screen dialog would be easier to implement than a dedicated fragment? |
2a1fc25
to
57b4e39
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It's nice seeing the transcript run by on by own phone. It already works great.
I added a bunch of comments below. The CI server also left a comment about a SpotBugs violation (that's new with the rebase :) )
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/ItemTranscriptRvAdapter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/ItemTranscriptRvAdapter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/ItemTranscriptRvAdapter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/ItemTranscriptRvAdapter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/TranscriptFragment.java
Outdated
Show resolved
Hide resolved
ui/chapters/src/main/java/de/danoeh/antennapod/ui/chapters/PodcastIndexTranscriptUtils.java
Outdated
Show resolved
Hide resolved
ui/chapters/src/main/java/de/danoeh/antennapod/ui/chapters/PodcastIndexTranscriptUtils.java
Outdated
Show resolved
Hide resolved
ui/chapters/src/main/java/de/danoeh/antennapod/ui/chapters/PodcastIndexTranscriptUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
address most comments, still have a few more to go
parser/transcript/src/main/java/de/danoeh/antennapod/parser/transcript/TranscriptParser.java
Outdated
Show resolved
Hide resolved
parser/transcript/src/main/java/de/danoeh/antennapod/parser/transcript/TranscriptParser.java
Outdated
Show resolved
Hide resolved
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/ItemTranscriptRvAdapter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/TranscriptFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/TranscriptFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/TranscriptFragment.java
Outdated
Show resolved
Hide resolved
ui/chapters/src/main/java/de/danoeh/antennapod/ui/chapters/PodcastIndexTranscriptUtils.java
Outdated
Show resolved
Hide resolved
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/TranscriptFragment.java
Outdated
Show resolved
Hide resolved
ui/chapters/src/main/java/de/danoeh/antennapod/ui/chapters/PodcastIndexTranscriptUtils.java
Outdated
Show resolved
Hide resolved
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/TranscriptFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/de/danoeh/antennapod/ui/screen/playback/TranscriptFragment.java
Outdated
Show resolved
Hide resolved
Thanks @tonytamsf! A few additional comments/thoughts:
|
There is still one quirk I don't understand. When opening the dialog for the second time while playing, it crashes. Error message: |
@ByteHamster Remind me of which podcast you are seeing this? Using Podcasting 2.0, I am not seeing this problem. |
I think I fixed this in 3d09a7c. The problem was that when loading was interrupted, it returned |
Looks ready for merging to me. Is there anything open from your side, tony? |
I have one more fix to make the dialog buttons not scroll and disable the refresh button during a refresh. |
I'm ready to merge @ByteHamster now, the refresh button is disabled during a refresh
|
We have addressed all of your comments except the font size, I think they are the same. |
The transcript button disappeared and it's also not in the overflow menu any-more for me 🫤 I also got a crash in the test build when attempting to stream Boostagram Ball for the first time (tapping the stream button after did work). Dunno if related to the chapters. EnvironmentAndroid version: 13 Crash infoTime: 16-05-2024 08:58:02 StackTrace
|
@keunes that crash looks unrelated, could you tell me which SHA is the AP debug build you are running? I suspect it is not the right build because the overflow button for transcript is not there |
Hi @tonytamsf |
Yeah, that's because we already have too many toolbar icons for small screens. Having it in the overflow menu is indeed hard to discover but I wanted to merge your PR before we start working on redesigning the player screen
That's a weird commit SHA. It doesn't belong to any commit in this PR. Probably some github internal stuff... I can confirm that it is indeed the artifact that can be downloaded here. I do see the button on my device. Maybe try another episode? |
@keunes I can see the transcript menu, and Podcasting 2.0 and Bostagram Ball works for me. Can you uninstall the debug AP and reinstall? Could you test again? Nothing has changed with the build. |
Ok, I identified the issue:
Not sure if the problematic scenario is supposed to be supported, but it'd be nice.
I'm just thinking: is it possible to switch around the share button and the transcript, while keeping the share visible if there's no transcript? (I.e., can we set a maximum number of icons for in the top bar and let Android figure out which one, depending on button relevance?) Reason:
I fully agree, but can we agree that this should be one of our priorities then? I'm a bit afraid that otherwise most of the podcasting 2.0 stuff will remain with little impact because we can't/don't want to expose them too much. |
I don't think supporting the edge case of importing an old database into the new app and playing before refreshing is a case we have to spend too much time on. Supporting that would complicate things without a real benefit.
We already let Android figure out the number. If it thinks there is space for the transcript, it already shows it. I don't think it ever does, though. Maybe on tablets. I'm not a big fan of displaying the share button in different places for different episodes. That feels a bit inconsistent. |
Like I said: the general public probably won't be affected. Also, I wouldn't say they are "different places"; the buttons and overflow menu are right next to each other, both in the end corner of the top app bar. I am really afraid, and I'm sorry to bring this up again, that the player screen redesign won't be prioritised (because it's a big & complex work, which will linger just like multiple queues) and then all the effort of implementing innovative changes in the ecosystem won't reach their potential. As we always say: workarounds are likely to stay. I would prefer to avoid that transcripts fall victim to this, even at the cost of tiny inconsistency for a minor amount of episodes. |
Considering other new features like podcast:socialinteract, we cannot add them all to the toolbar icons. Even if we decided we want to move the "share" button. If you want to make it more prominent, I would rather add it to the buttons below the cover. It does make the cover smaller, but only if chapters or comments are supported for that episode. Next to chapters and description is a more logical place than next to sleep timer and favorite anyway. |
Does having vertical buttons along cover has been considered ? I don't feel it's a good idea because cover won't be centered but it could be a solution to fit more buttons |
I am in favor of merging this PR as is, to get it in the hands of real users during beta. I am pretty certain they will have one to two bugs or feature requests before we go live. |
Thanks! I will rebase and merge the transcript branch now. |
fix #4935
Description
Display contents of podcast:transcript tag as a full dialog view using RecylerView.
Yet to do
Future
Image & Video
Version 0.9
Follow audio checkbox
audio.follow.mp4
Version 0.1
transcript-window-480.mov